Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

<base href="/"> breaks SVG IRIs and element CSS URLs #1224

Closed
0x24a537r9 opened this issue Feb 18, 2016 · 18 comments · Fixed by #1544
Closed

<base href="/"> breaks SVG IRIs and element CSS URLs #1224

0x24a537r9 opened this issue Feb 18, 2016 · 18 comments · Fixed by #1544

Comments

@0x24a537r9
Copy link
Contributor

Unfortunately, the <base href="/"> tag in modules/core/server/views/layout.server.view.html breaks SVG IRIs and element CSS URLs.

For those very reasonably unsure what I'm referring to:

  • SVG IRIs are the formal name for SVG attribute values like url(#myElement), used throughout SVGs for referencing other elements usually defined in <defs> tags. They are the only way to do a large number of things in SVGs including filters/manipulations, text paths, marker-start/-mid/-end (the way to make lines into arrows). The SVG spec 25 different SVG elements that can use IRIs--many of which only work with IRIs.
  • Similarly, element CSS URLs are commonly used for a number SVG-like effects on regular HTML elements, like mask, clip-path, and filter. These only work with relative URLs.

The problem arises because, as mentioned in this article, <base href="/"> will apply to all URL/IRIs in the document, including SVG IRIs and CSS URLs, which basically always reference a relative element id on the same page.

Take this jsfiddle. Note the happy little triangle making the <polyline> into an arrow. Now look at the same fiddle with <base href="/">. The arrow is no longer there because the IRI is now pointing incorrectly to https://jsfiddle.net/#Triangle. You can tell by hovering over the blue-linked url("#Triangle") style value in a Chrome style inspector on the <polyline>.

Unfortunately, there's only one way to override the <base> tag, and it only works in some situations and is pretty unpleasant. This jsfiddle demonstrates that you can instead use an absolute URL in the IRI, since <base> tags only apply to relative URLs. Unfortunately, this has its downsides:

  1. This forces you to add logic in your template to insert the absolute path to the current page into the URL, which is annoying if the URL is dynamic, and frankly, kinda ugly. It's also very fragile, breaking if the page's URL changes, like when you click "Run" on the same jsfiddle, which just changes the URL. And it only gets more complicated if you have different URLs for the same page (e.g. vanity URLs).
  2. This workaround doesn't work at all if you don't have the ability to insert the absolute path into the SVG IRI or CSS URL (perhaps they are generated by a third-party library, or are simply third-party content in the case of SVG artwork).
  3. This workaround also doesn't work at all if you have multiple elements with the same id in separate <svg> elements on the same page and want to refer to them. For example, there is simply no way to make this jsfiddle work with a <base> tag and absolute URLs, although it's clearly a valid use (since <svg> elements kind of have their own documents, so the two ids aren't actually colliding).

So what can we do? Well, in this StackOverflow question, a user asks how to override <base>, to which one user responded (back in 2012):

The base tag is normally not needed these days. It is better to use server-side technologies that let you construct addresses from one or more base addresses. So quite possibly the best approach is to get rid of the tag.

It took me a good 30 mins of probing to get to the bottom of this, and I'd consider myself an expert with JS/HTML/CSS. For less experienced engineers, this could be a real tough bug to track down. And with the rise of visualization libraries like D3, SVG bugs like this are only going to come up more and more often. Since all of the page resources are controlled by a server-side template anyway, I think we should strongly consider prioritizing correctness over convenience and remove the <base> tag entirely, switching to fully-qualified path names for external resources.

Sorry for the essay 😅 Thoughts?

@pgrodrigues
Copy link
Contributor

I had this same problem which unfortunately took me way more than 30 mins to realize what the cause was and ended up answering my own question in SO. I changed the SVG marker paths to the absolute ones, but I have to agree that it is not a robust workaround.

@lirantal lirantal self-assigned this Feb 20, 2016
@lirantal lirantal added this to the 0.5.0 milestone Feb 20, 2016
@lirantal
Copy link
Member

@0x24a537r9 and @pgrodrigues thanks for the feedback.
@0x24a537r9 if removing the base tag, what kind of absolute paths do we need to setup?
and would you like to consider submitting a PR

@0x24a537r9
Copy link
Contributor Author

I'm happy to start a PR with guidance from someone more familiar with the codebase. I suspect that I would only need to change the paths in modules/core/server/views/layout.server.view.html to be absolute (fairly easy I believe) but, while I'm quite proficient with HTML/CSS/JS, I've only just started using Angular and MEAN.JS two weeks ago, so I'm still getting a feel for the codebase.

Given how far-reaching something like <base href="/"> can be, I'd like to hear from someone with deep architectural knowledge--more familiar than I with the ins-and-outs of the framework--if there are any other places likely to need fixing (e.g. in dynamic resource URLs like profile images, etc.). I'd hate for my first PR on the project to break "all of the things".

@0x24a537r9
Copy link
Contributor Author

Slight complication... when I try removing the <base> tag, I receive this warning:

Uncaught Error: [$location:nobase] $location in HTML5 mode requires a tag to be present!
http://errors.angularjs.org/1.3.20/$location/nobase

Some Googling lead me to this Angular doc, which recommends using relative URLs with a <base> tag to ease migrations from root contexts (e.g. https://myapp.com/) to sub-contexts (e.g. https://myapp.com/subapp/) when in HTML5 mode. It notes that one can:

configure $locationProvider to not require a base tag by passing a definition object with requireBase:false to $locationProvider.html5Mode()

but that

removing the requirement for a <base> tag will have adverse side effects when resolving relative paths with $location in IE9.

Is IE9 even in MEAN.JS's intended support profile? Thoughts on whether we should still remove the <base> tag?

Also, as an aside, it looks like there are a few other references that need updating outside of modules/core/server/views/layout.server.view.html, but they appear to be minimal and relatively easy to update. Many are simply profileImageURLs, and the rest are literal paths.

@0x24a537r9
Copy link
Contributor Author

Ok, I went ahead and started PR #1230, manually updating all references I found via:

egrep -r "(resource\('|([Uu]rl|[Hh]ref|[Ss]rc)[=:])" modules/
egrep -r "'(api|modules)/" modules/

This got me most of the way there, and a few more manual tweaks revealed in manual testing got me to the point where everything seems to work as before in a local demo. All the server and e2e tests work, but unfortunately I'm having trouble with the client tests, where a large number fail before even getting to the tests' expect calls with errors like:

PhantomJS 1.9.8 (Linux 0.0.0) Articles Controller Tests vm.save() as create should send a POST request with the form input values and then locate to new object URL FAILED
    Error: Unexpected request: GET /modules/core/client/views/home.client.view.html
    No more request expected
        at $httpBackend (/home/cbehar/dev/elf/public/lib/angular-mocks/angular-mocks.js:1264)
        at sendReq (/home/cbehar/dev/elf/public/lib/angular/angular.js:9695)
        at /home/cbehar/dev/elf/public/lib/angular/angular.js:9406
        at processQueue (/home/cbehar/dev/elf/public/lib/angular/angular.js:13318)
        at /home/cbehar/dev/elf/public/lib/angular/angular.js:13334
        at /home/cbehar/dev/elf/public/lib/angular/angular.js:14570
        at /home/cbehar/dev/elf/public/lib/angular/angular.js:14386
        at /home/cbehar/dev/elf/public/lib/angular-mocks/angular-mocks.js:1562
        at /home/cbehar/dev/elf/modules/articles/tests/client/articles.client.controller.tests.js:90
        at invoke (/home/cbehar/dev/elf/public/lib/angular/angular.js:4219)
        at workFn (/home/cbehar/dev/elf/public/lib/angular-mocks/angular-mocks.js:2475)
    undefined

All of the articles and chat controller tests fail with an unexpected request to home.client.view.html and all of the password controller tests fail with an unexpected request to 404.client.view.html. Everything else works fine, and all the functionality seems fine when manually testing with localhost:3000. I've spent the past hour or so tracing through the code to find where the GET request is coming from, but my Angular/Karma-foo is still weak. Any ideas?

@lirantal
Copy link
Member

@codydaig, @rhutchison what do you think?

@lirantal
Copy link
Member

@0x24a537r9 seems that karma.conf.js needs to be modified so that the ngHtml2JsPreprocessor directive has the correct stripPrefix rule for the paths.

@trendzetter
Copy link
Contributor

Can it be we need something similar to fix the tests for this pr #1207 ? Very same fail:

PhantomJS 1.9.8 (Linux 0.0.0) Articles Controller Tests vm.save() as create should send a POST request with the form input values and then locate to new object URL FAILED

    Error: [$injector:modulerr] Failed to instantiate module ng due to:

    TypeError: 'undefined' is not an object (evaluating 'Function.prototype.bind.apply')

@usta
Copy link

usta commented Feb 25, 2016

@trendzetter i'm not sure if you read my comments. PhantomJS 1.x series DOESN'T SUPPORT Function.prototype.bind.apply I have wrote this about 1 million times. If we want to see out tests doesn't fail. We have to DO : ( I will not write again these lines this will be my last writing about this Function.prototype.bind.apply )
1- on serverside we need to upgrade to phantomjs to "phantomjs-prebuilt": "~2.1.4", and update karma-phantomjs-launcher to "karma-phantomjs-launcher": "~1.0.0",
2- after first step done we need to rewrite chat module's tests or we need to remove use strict from this two : socket.io.client.service.tests.js and chat.client.controller.tests.js

BUT These 2 are just serverside test falsepossitives so it is not this PRs problems

@0x24a537r9
Copy link
Contributor Author

@trendzetter I don't believe your issue is in any way related to this one--the errors are completely different except that they both involve undefined, which is a fairly tenuous connection.

Update: unfortunately I haven't had time yet after work to try stripPrefix, but I hope to tonight and will post back with results.

@0x24a537r9
Copy link
Contributor Author

Hm, unfortunately neither stripPrefix nor prependPrefix worked to solve the problem.

@lirantal am I correct in assuming that you thought the failure might be related to ngHtml2JsPreprocessor because of this thread? It certainly sounds like the same issue, and after reading more about $httpBackend I can see why ngHtml2JsPreprocessor might no longer be correctly setting up the $templateCache for ui-router now that I've prepended / to each templateUrl. Shouldn't prependPrefix have thus fixed it though if that were the case?

I'll continue trying to debug.

@lirantal
Copy link
Member

That's what I thought of but I hadn't done any work to investigate that path.
Thanks for trying to help out on this.

@simison
Copy link
Member

simison commented Mar 11, 2016

I've also been looking for a solution to this. So far I've been able to get rid of errors about unexpected home.client.view.html requests by adding this:

beforeEach(module(function($urlRouterProvider) {
  $urlRouterProvider.deferIntercept();
  // Tested, no dice:
  //$urlRouterProvider.otherwise(function(){return false;});
}));

But that will break tests testing $urlRouterProvider.rule() stuff found in PR #1238

Reading:

@Wuntenn
Copy link
Contributor

Wuntenn commented Apr 19, 2016

If you think about the components that cause the problem:

none of these are meanjs specific and point more toward AngularJS itself.

Googling: "angular $location base url svg" we find that the angular community have looked into it

angular/angular.js#8934

They've introduced a way to use html5mode without requiring a base path. This fixes issues with the SVG path however cuts support for browsers which do not have support for history push.

However from further searches I was able to find:

http://stackoverflow.com/questions/19742805/angular-and-svg-filters

Which should fix the problem by putting absolute references into the SVG (hurdle identified as bullet pt 2 in @0x24a537r9 comment above).

It may be unfeasible to edit generated SVG by hand to take advantage of this however a gulp task could automate this process allowing both SVG and HTML5mode support. The only downside would be the 2 SVG's with the same ID problem (bullet 3).

Still, two ways to get around the problem with minimal change meanjs or svg give us options.

@ilanbiala
Copy link
Member

@0x24a537r9 have you looked at a gulp task to do this?

@lirantal
Copy link
Member

@Wuntenn would you like to complete the work in this issue and relevant PR that is open (some tests fail there)?

@Wuntenn
Copy link
Contributor

Wuntenn commented Jul 24, 2016

Sure. Gimmie a sec - It should be a one liner (assuming it works as intended)

@lirantal
Copy link
Member

@meanjs/contributors anybody wishes to take a look here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants